-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix transformation host selection #379
Fix transformation host selection #379
Conversation
@@ -43,9 +43,9 @@ def self.get_runners_count_by_ems(ems, factory_config) | |||
|
|||
def self.get_transformation_host(task, factory_config, handle = $evm) | |||
ems = handle.vmdb(:ext_management_system).find_by(:id => task.get_option(:destination_ems_id)) | |||
ems_max_runners = ems.custom_get('Max Transformation Runners').to_i || factory_config['ems_max_runners'] || 1 | |||
ems_max_runners = ems.custom_get('Max Transformation Runners') || factory_config['ems_max_runners'] || 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that ems_max_runners
is an integer and we should not have to call to_i
on line 48 when you are going to evaluate it. Also 10
should be a class constant DEFAULT_EMS_MAX_RUNNERS
.
module Common
class Utils
DEFAULT_EMS_MAX_RUNNERS = 10
def initialize(handle = $evm)
You could do something like this:
ems_max_runners = (ems.custom_get('Max Transformation Runners') || factory_config['ems_max_runners'] || DEFAULT_EMS_MAX_RUNNERS).to_i
A better refactoring would be to move this logic into a method where you could potentially log where the value was coming from if you felt that was important.
def ems_max_runners
if ems.custom_get('Max Transformation Runners')
<log message>
ems.custom_get('Max Transformation Runners').to_i
elsif factory_config['ems_max_runners']
<log message>
factory_config['ems_max_runners'].to_i
else
<log message>
DEFAULT_EMS_MAX_RUNNERS
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ems_max_runners
is not an integer when it is a custom attributes, because custom attributes are stored as strings.
Anyway, it's a good idea to create a method, as the message will help debugging.
@@ -32,8 +32,8 @@ def main | |||
@handle.log(:info, "Transformation - Started On: #{start_timestamp}") | |||
|
|||
destination_ems = @handle.vmdb(:ext_management_system).find_by(:id => task.get_option(:destination_ems_id)) | |||
max_runners = destination_ems.custom_get('Max Transformation Runners').to_i || factory_config['max_transformation_runners_by_ems'] || 10 | |||
if Transformation::TransformationHosts::Common::Utils.get_runners_count_by_ems(destination_ems, factory_config) >= max_runners | |||
max_runners = destination_ems.custom_get('Max Transformation Runners') || factory_config['max_transformation_runners_by_ems'] || 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concepts apply here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using the embedded method to not duplicate code. A separate method is even a better idea because the default value lives in a single method.
factory_config['ems_max_runners'].to_i | ||
else | ||
handle.log(:info, "Using default max transformation runners: #{DEFAULT_EMS_MAX_RUNNERS}") | ||
DEFAULT_EMS_MAX_RUNNERS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value was 1 in the get_transformation_host
previously here https://github.com/ManageIQ/manageiq-content/pull/379/files#diff-44115b1bf3b0e7b502024d527948ef6dL46
now DEFAULT_EMS_MAX_RUNNERS = 10
which is what VMTransform
expects.
Does the value need to be flexible?
If so I would suggest adding another option to the end of the ems_max_runners
method.
Example: def self.ems_max_runners(ems, factory_config, handle = $evm, max_runners = DEFAULT_EMS_MAX_RUNNERS)
and override the value from get_transformation_host
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Indeed, it may change over time. And we may see a more complex heuristic to determine max_runners. I changed it also for host_max_runners.
@@ -16,6 +19,19 @@ def self.get_runners_count_by_host(host, handle = $evm) | |||
handle.vmdb(:service_template_transformation_plan_task).where(:state => 'active').select { |task| task.get_option(:transformation_host_id) == host.id }.size | |||
end | |||
|
|||
def self.host_max_runners(host, factory_config, max_runners = DEFAULT_HOST_MAX_RUNNERS, handle = $evm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fdupont-redhat Is this just a Utility class which only has class methods that you want to embed into other classes. We don't need to have an initialize or the call at the bottom if you are going to just call it from other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this as well as there is an empty main
method defined. I'm good with including those changes here if appropriate but that refactoring could be in a separate PR as well since it is unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's just a utility class. It is meant to be added as an embedded method. Should I remove it ? It's a bit out of scope for this PR. Maybe another cleanup PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I removed the unneeded parts: initialize, main and call at end of file.
Checked commits fabiendupont/manageiq-content@83b3926~...70f5df7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
ems_cur_runners = get_runners_count_by_ems(ems, factory_config) | ||
transformation_host_hash = ems_cur_runners < ems_max_runners ? eligible_transformation_hosts(ems, factory_config).first : {} | ||
transformation_host_hash = ems_cur_runners < ems_max_runners(ems, factory_config) ? eligible_transformation_hosts(ems, factory_config).first : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you planning to change this call to ems_max_runners
to pass 1
as the third parameter to override max_runners
? That would maintain the previous logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. It was 1
just because I didn't know what could be a reasonable value. Now that RHV perf team has said that 10 should be the default per host, I'm setting it to 10 at the EMS level. This would allow 1 conversion host per EMS by default. Then user can override it using custom attribute.
…st_selection Fix transformation host selection (cherry picked from commit b2982c6) https://bugzilla.redhat.com/show_bug.cgi?id=1610054
Gaprindashvili backport details:
|
The comparison fails because
nil.to_i
equals to 0. This leads to endless loop on the AcquireTransformationHost and VMTransform states.Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1600152